Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BearSSL client and server, support true bidir, lower memory, modern SSL #4273

Merged
merged 28 commits into from
May 15, 2018

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Feb 2, 2018

-edit- No longer WIP, seems stable enough for review and testing use outside my own lab -edit-

As discussed in #3490, axTLS has some issues with not checking for OOM, is unidirectional (i.e. some hacks required to support bidir MQTT protocol/etc.), and uses lots of memory.

This patch takes BearSSL, ports it to the ESP8266, and creates two new classes to replace the WiFiServerSecure and WiFiClientSecure classes. BearSSL memory requirements are much lower than axTLS when doing bidirectional (only ~24KB for a client, ~5.5KB for a server) connections since the receive and transmit buffer sizes are independently configurable, allowing for both a bidir SSL client and bidir SSL server to be run at the same time with memory to spare for your own application. It also does not do any memory allocations at operation time, so you're not likely to get as many random crashes as axTLS when tight on memory. Finally, BearSSL supports a much wider variety of modern ciphers (and does not do the completely broken RC4, for example) and certificate types.

BearSSL is MIT licensed by the author.

While the patch here does work and can be used as both a SSL server and Client, it's still a WIP as I expect the API to change and grow, as well as more advanced testing by real users.

The Server/Client interface is the same as the existing classes, but the SSL-specific bits are different. This applies to the way certificates are validated (they are ALWAYS checked against a trust anchor/hardcoded cert you supply), and how SSL parameters are set amongst other things, so it's not as simple as just replacing WiFiClientSecure w/WiFiClientBearSSL.

The new classes are called (for now) WiFiClientBearSSL/WiFiServerBearSSL.

See the new example bearssl/bearssl.ino for a quick example. See https://bearssl.org for more BearSSL info as well as the tools to generate your own trust anchor structures for your own apps.

I'm including a precompiled bearssl.a library for now, but the changes to the original sources are quite trivial and will be put up at least in my own repo if not integrated w/the main BearSSL code if they're acceptable to the author.

https://github.com/earlephilhower/bearssl-esp8266 is the port sources.

Thanks very much to the BearSSL author @pornin for his assistance in the original issue.

@earlephilhower

This comment has been minimized.

@pornin

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@igrr

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@igrr

This comment has been minimized.

@d-a-v

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@earlephilhower

This comment has been minimized.

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Feb 21, 2018

Great work @earlephilhower ! I am adopting your bearssl port in my AsyncTCP fork, and have similar observations on stack usages -- starting at 33KB of free heap space before making any connection, it quickly drains down to 2KB before a x509 decoder can be created. (the default 16KB recv buffer is the main culprit).

But I have an interesting observation during debugging:

  • Because of how AsyncTCP works, data processing happens directly in the LWIP callback context, when the stack pointer is inside the "sys" context, rather than "g_cont";
  • I found the sys stack usage is rather low in LWIP callbacks, only around 300 bytes
  • Meanwhile, according to https://bbs.espressif.com/viewtopic.php?t=8879 the sys stack is rather large, around 8KB
  • It means that, we have over 7KB of free stack space lying around unused!

Is it possible to repurpose the free sys stack to satisfy the 4.5KB heap needs of bearssl?

@earlephilhower
Copy link
Collaborator Author

Very interesting idea, @Adam5Wu!

First of all, you can/should set a smaller transmit buffer size. There's no reason to allow a full 16KB transmit buffer unless you really need the highest bandwdth SSL transmit. Try the ~900 bytes transmit size I default to, it works but will break up >512b unencrypted packets into multiple 512b encrypted ones.

On receive the same logic applies. If you're not expecting, say, >2KB POSTs from a web client, then set server receive buffers smaller as well. SSL clients, though, you're kind of stuck unless you can only generate HTTP RANGE GETs..OTW 16KB receive buffers are required.

Those will save you tons of heap, but none of the stack which still needs many KB to work (not a dig on BearSSL, the encryption algorithms themselves have large temporary memory needs).

To do what you're suggesting correctly we'd need to get to the SYS context for bearssl calls, and I don't know how to do that. Do you?

To do it incorrectly (as in super-hackish) all you need to be able to do is get the end of the SYS stack. Pass that address -4.5KB in to the synthetic stack function, and it'll "just work" assuming the last 4.5KB of SYS stack aren't used. Again, I'm not sure how to grab the SYS context info as I've never thought about it...if we check before calling a BearSSL API that the space we say is available it should be fine...

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Feb 21, 2018

Checkout libraries/ESP8266WiFi/src/include/ClientContext.h
I think the _recv() function is a callback from LWIP. Print the stack pointer:

    register uint32_t *sp asm("a1");
    Serial.printf("current stack pointer = %p\n", sp);

You should see something like 3ffffcXX, and it seems this stack can go as deep as 3fffe000.

I have tried to modified my local copy of your bearssl port, effectively undo the alternative heap changes, and it seems I am able to complete a connection under AsyncTCP, with 4.5KB more free heap. :)

I think it is possible to enjoy the benefit of extra sys stack without having to fully embrace AsyncTCP.
(Disclaimer: my understand of TLS protocol is limited) I think the most complex crypto operations happen at handshake, which also involves the heavy stack usage. So it is possible to hook into ClientContext, and ONLY intercept the hankshake part of the traffic and perform those most expensive operations in the callback context. After handshake is completed, we can continue to use the current (non-async) workflow. And because regular send/receive do not involve heavy crypto, there is no need for extra stack.

@earlephilhower
Copy link
Collaborator Author

The system stack is spelled out, not explicitly, in the app.ld file. There's a line after the dram0_0_seg that's

/* __stack = 0x3ffc8000; */

So that's legit, but what we need to make it safe to use outside of the sys context in appspace is the location they stuff the stack pointer on a context switch. Since we're going to a new SDK, even, the comments on the espressif board may not hold true any longer. :(

Also, FWIW, the elliptic curve crypto takes more stack space than the x509 decode. IT's actually not the x509 portion, but the RSA 4Kbit signing/checking part of the certificate validation that eats up space early in the process. You can pull down the stack space a little bit (~1.5KB IIRC) by disabling all the elliptic curve support, but honestly I'd rather have a "works and can validate with darn near anything" than the extra 1.5KB.

In case you didn't find it already, to disable the stack stuff just go to src/inner.h, go down to the bottom of the file, and replace the STACK_PROXY_ALLOC() from ESP8266 with the non-ESP8266 one. That'll just define variables in the function like standard GCC.

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Feb 21, 2018

ECDSA support is the main reason I pursue bearssl, so I am not going to give that up! 😄

I may be wrong, but to my understanding, RSA or ECDSA, they ONLY happen at handshake stage, when certificates are validated and shared secrets are exchanged. After that point, everything else only involves symmetric key block/stream cipher.

So my point is, during the short timespan of "after TCP connected" --> "handshake done", hook into ClientContext and perform all bearssl "heavy crypto" in the callback context, let it use the sys context stack, which can go up to 8KB. (By "use" I mean "natrual" use, not through the proxy)

Once handshake completes, the rest of bearssl operations can happen in cont context (as they do now). Because only "simple crypto" will be invoked (block/stream cipher, no RSA/ECC), the stack usage will not be high, so again, we can let it use the cont context stack natrually without proxy.

And thanks for the hint about stack proxy macros, that is just what I did. 😃

ta->flags |= BR_X509_TA_CA;
}

delete dc; // Done with x509 decoder
Copy link
Contributor

@Adam5Wu Adam5Wu Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point pk still holds reference to structure inside dc, so it cannot be freed here.
Issue became visible when add some debug prints after this line corrupted the public key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this! I'll throw a new commit up soon, simple fix (you've probably already done it in your own copy).

@earlephilhower
Copy link
Collaborator Author

About the 2ndary stack, there's talk of using this for other things in the Arduino side as well on the maintainers gitter. We may get some official (or at least better) info on how much can safely be used.

I'll need to see how the ctx switching is handled to see if we can "easily" pop just the certificate validation in the SYS stack. The easiest way, though, would be to use the setup as-is and point that 4.5K buffer to SYS_STACKEND-4,.5KB and call it a day. Assuming that stack space is always available anyway (because the SYS stack is overallocated), that's just as safe as handling things while in SYS.

I think you're correct on the stack usage differences. Seems like only the key exchange is done using ECC or RSA, everything else is almost trivial AES or other simple block codes.

Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was leaving some comments and realized that the PR is already merged. Feel free to ignore...

bool begin(String host, uint16_t port, String uri, String httpsFingerprint);
// Use BearSSL for secure HTTPS connection
bool begin(String url, const uint8_t httpsFingerprint[20]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we pass the fingerprint as String? If the intention is to disambiguate between two implementations of TLS, then it's a really non-obvious way to do this. Introducing a function with different name (beginBearSSL?) and keeping the argument type might be an option.

@@ -19,43 +19,5 @@
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The copyright header can now be removed from this file.

Serial.printf("Connected!\n-------\n");
client->write("GET ");
client->write(path);
client->write(" HTTP/1.0\r\nHost: ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a fair few issues users have been advised to use HTTPClient library instead of rolling their own HTTP request code. Does it make sense to update the example to do the same?

void loop() {
Serial.printf("\n\n\n\n\n");

yield();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this yield really needed?

#ifndef _BEARSSLHELPERS_H
#define _BEARSSLHELPERS_H

#include <bearssl/bearssl.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to avoid pulling in third party libraries into sketch namespace. This can be done is a way similar to the way LwIP and axTLS are currently hidden from users.

path += "/";
}

String tblName = path + "ca_tbl.bin";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the name hardcoded?

#include <Arduino.h>
#include <bearssl/bearssl.h>

// Virtual base class for the certificate stores, which allow use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a base class for certificate stores, why is it specific to BearSSL? If we add a similar class for AxTLS, would it be derived from BearSSL one?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello:
have a stable version with this changes?

This was referenced May 29, 2018
@arnolde
Copy link

arnolde commented Sep 18, 2018

So a newbie who has been using:

#include <ESP8266WiFi.h>
#include "ESP8266HTTPClient.h"
[...]
HTTPClient http;
http.begin(url, fingerprint);

can simply switch to bearSSL exactly how...?
(currently not (knowingly) using any other libraries)

@earlephilhower earlephilhower deleted the bearssl_wip branch September 30, 2018 17:53
@eiannone
Copy link

@arnolde , see the example here: https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266HTTPClient/examples/BasicHttpsClient/BasicHttpsClient.ino

@alirezash12345
Copy link

alirezash12345 commented Jun 13, 2020

hello every one I want to use setBufferSizes for BearSSL Secure Server Class which is mentioned here:
https://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/server-class.html
I am not sure how to do it and I couldn`t find any document about this application. can annyone help me about this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.